feat(IamAssumeAuthenticator): introduce a new authenticator #211

pyrooka · 2024-10-08T13:39:10Z

This commit introduces the new IAMAssumeAuthenticator which will fetch an IAM access token using the IAM get_token operation's "assume" grant type. The resulting access token allows the application to assume the identity of a trusted profile, similar to the "sudo" feature of Linux.

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

padamstx

LGTM, although I was able to demonstrate my reviewer's prowess by finding a couple of small typos 😂

Authentication.md

ibm_cloud_sdk_core/authenticators/iam_assume_authenticator.py

ibm_cloud_sdk_core/get_authenticator.py

ibm_cloud_sdk_core/token_managers/iam_assume_token_manager.py

test/test_iam_assume_token_manager.py

test/test_utils.py

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

padamstx

Still LGTM

dpopp07

Looks good! Just want to make sure we're handling the refresh token consistently with Node, and had one other question for my own edification

dpopp07 · 2024-10-08T22:47:15Z

ibm_cloud_sdk_core/authenticators/container_authenticator.py

@@ -66,6 +66,7 @@ class ContainerAuthenticator(IAMRequestBasedAuthenticator):

    def __init__(
        self,
+        *,


What is this change doing? Appreciate your patience with my lack of Python knowledge 😅

Arguments before the asterisk are positional argument, so they can be passed like call_function(1, 2). Arguments after the * become keyword only, so they cannot be used in the previous way, only like this: call_function(a=1, b=2). It's not advised to have too many positional arguments because for example to specify the last one, you have to specify all the previous one too. Since they are used by their position not their keyword.
TLDR; there is a new pylint rule that complains about this.

Yes, this changes the behavior a little, but I don't think this would be a breaking change. If we start getting negative feedback, we can restore the original behavior pretty easily and exclude the affected functions/methods from the linter.

dpopp07 · 2024-10-09T14:57:43Z

test/test_iam_assume_token_manager.py

+    assert token_manager.request_payload.get('account_id') is None
+
+    # The final result should be the other access token, which belong to the "assume" request.
+    assert access_token == OTHER_ACCESS_TOKEN


Will you add an assertion that the refresh token is not set?

Good catch. It turned out I only made this change in the authenticator and not in the token manager, so all these properties were accessible: refresh_token, client_id, client_secret

So is it also the case that the client ID and secret will, if set, be used in the IAM delegate request but not in the assume request?

pyrooka · 2024-10-09T16:41:13Z

ibm_cloud_sdk_core/authenticators/iam_assume_authenticator.py

-            raise AttributeError(f"'IAMAssumeAuthenticator' has no attribute '{name}'")
+            raise AttributeError(f"'{self.__class__.__name__}' has no attribute '{name}'")


Now we won't have to care about changing this if we decide to rename the class. :)

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

…henticator Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

padamstx

Latest changes look good.

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

# [3.22.0](v3.21.0...v3.22.0) (2024-10-15) ### Features * **IAMAssumeAuthenticator:** introduce a new authenticator ([#211](#211)) ([29a8eb7](29a8eb7))

ibm-devx-sdk · 2024-10-15T14:18:31Z

🎉 This PR is included in version 3.22.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

pyrooka force-pushed the nb/iam-assume-authenticator branch 2 times, most recently from c7a7a40 to acf5561 Compare October 8, 2024 13:49

feat(IamAssumeAuthenticator): introduce a new authenticator

99ea907

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

pyrooka force-pushed the nb/iam-assume-authenticator branch from acf5561 to 99ea907 Compare October 8, 2024 16:18

pyrooka added 2 commits October 8, 2024 19:18

chore: add back removed properties

29b5dc5

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

chore: disallow unsupported parent methods, fix (new) linter issues

83e423a

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

pyrooka requested review from dpopp07 and padamstx October 8, 2024 19:18

padamstx approved these changes Oct 8, 2024

View reviewed changes

pyrooka added 2 commits October 9, 2024 13:51

chore: fix typos

97e4d3b

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

chore: iam assume token manager test improvements

36425f9

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

pyrooka force-pushed the nb/iam-assume-authenticator branch from 2d1548c to 36425f9 Compare October 9, 2024 13:10

padamstx approved these changes Oct 9, 2024

View reviewed changes

dpopp07 approved these changes Oct 9, 2024

View reviewed changes

pyrooka force-pushed the nb/iam-assume-authenticator branch from 66a9949 to 983e335 Compare October 9, 2024 16:41

pyrooka commented Oct 9, 2024

View reviewed changes

fix: disable unsupported attributes in the new token manager class

872c678

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

pyrooka force-pushed the nb/iam-assume-authenticator branch from 983e335 to 872c678 Compare October 9, 2024 16:43

pyrooka added 2 commits October 9, 2024 20:52

fix: make sure the delegate has the same ssl config

a35431e

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

chore: use temporary IAM authenticator to validate our IAM delegate

481f59e

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

pyrooka force-pushed the nb/iam-assume-authenticator branch from f9617ee to e8f1a42 Compare October 9, 2024 19:34

chore: change a few things, fix a few things...

999ce92

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

pyrooka force-pushed the nb/iam-assume-authenticator branch 2 times, most recently from ae7ead8 to db84a5e Compare October 10, 2024 16:01

pyrooka added 2 commits October 10, 2024 18:11

chore: disable all setter methods in the assume token manager and aut…

be503b6

…henticator Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

chore: update .secrets.baseline

85bc3f9

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

pyrooka force-pushed the nb/iam-assume-authenticator branch from db84a5e to 85bc3f9 Compare October 10, 2024 16:11

padamstx approved these changes Oct 11, 2024

View reviewed changes

chore: a few small adjustments to increase our code coverage

5fa39f0

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>

pyrooka force-pushed the nb/iam-assume-authenticator branch from 5204511 to 5fa39f0 Compare October 15, 2024 13:49

pyrooka merged commit 29a8eb7 into main Oct 15, 2024
4 checks passed

pyrooka deleted the nb/iam-assume-authenticator branch October 15, 2024 14:14

ibm-devx-sdk pushed a commit that referenced this pull request Oct 15, 2024

chore(release): 3.22.0 release notes

ea2d733

# [3.22.0](v3.21.0...v3.22.0) (2024-10-15) ### Features * **IAMAssumeAuthenticator:** introduce a new authenticator ([#211](#211)) ([29a8eb7](29a8eb7))

ibm-devx-sdk added the released label Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(IamAssumeAuthenticator): introduce a new authenticator #211

feat(IamAssumeAuthenticator): introduce a new authenticator #211

pyrooka commented Oct 8, 2024

padamstx left a comment

padamstx left a comment

dpopp07 left a comment

dpopp07 Oct 8, 2024

pyrooka Oct 9, 2024 •

edited

Loading

dpopp07 Oct 9, 2024

pyrooka Oct 9, 2024

dpopp07 Oct 9, 2024

pyrooka Oct 9, 2024

padamstx left a comment

ibm-devx-sdk commented Oct 15, 2024

		raise AttributeError(f"'IAMAssumeAuthenticator' has no attribute '{name}'")
		raise AttributeError(f"'{self.__class__.__name__}' has no attribute '{name}'")

feat(IamAssumeAuthenticator): introduce a new authenticator #211

feat(IamAssumeAuthenticator): introduce a new authenticator #211

Conversation

pyrooka commented Oct 8, 2024

padamstx left a comment

Choose a reason for hiding this comment

padamstx left a comment

Choose a reason for hiding this comment

dpopp07 left a comment

Choose a reason for hiding this comment

dpopp07 Oct 8, 2024

Choose a reason for hiding this comment

pyrooka Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

dpopp07 Oct 9, 2024

Choose a reason for hiding this comment

pyrooka Oct 9, 2024

Choose a reason for hiding this comment

dpopp07 Oct 9, 2024

Choose a reason for hiding this comment

pyrooka Oct 9, 2024

Choose a reason for hiding this comment

padamstx left a comment

Choose a reason for hiding this comment

ibm-devx-sdk commented Oct 15, 2024

pyrooka Oct 9, 2024 •

edited

Loading